fix(agents): reclaim zombie sandbox containers and create them lazily#4513
Merged
Conversation
Contributor
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4513 +/- ##
===========================================
+ Coverage 32.48% 56.69% +24.20%
===========================================
Files 216 359 +143
Lines 18368 39329 +20961
Branches 6478 11052 +4574
===========================================
+ Hits 5967 22296 +16329
- Misses 12369 16962 +4593
- Partials 32 71 +39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
Electric Agents Mobile BuildLocal mobile checks ran for commit The EAS Android preview build was skipped because the |
Users opening the desktop app found 15+ electric-sbx-* containers running that they never asked for. Several compounding bugs: - The boot sweep only removed *exited ephemeral* leftovers, but crash/ quit leftovers are RUNNING (PID 1 is an infinite sleep loop), so it never reclaimed anything real. Containers now carry an owner-pid label; the sweep reclaims running orphans whose owner is dead (remove ephemeral / stop persistent), consults an in-container adoption marker so a live process that reattached a dead creator's container is never swept, and is awaited at boot so it can't race reattaches. - The debounced idle teardowns are unref'd timers that died with the process: every graceful quit leaked the recently-active containers as running zombies. Runtime shutdown now flushes pending teardowns (BuiltinAgentsServer.stop), leaving live-leased entries to their own dispose or the next boot's sweep. - A failed post-start init (mkdir exec) left a started container that was never registered - invisible to dispose and, while running, to the sweep. Creation now verifies the init exit code and removes the container on any failure. Fixing this exposed that isNameConflict() treated any HTTP 409 as a lost create race (exec on an exited container is also 409), silently "reattaching" to a removed container; it now matches the daemon's name-conflict message. - Sandbox creation was eager on every claimed wake, so a reconnect backlog of trivial wakes (cron ticks, bookkeeping) stampeded the daemon with containers. The docker profile now returns a lazySandbox wrapper that defers the provider factory to first actual use. Terminal reclaim without use goes through reclaimDockerSandboxByKey (no create-to-delete), spawn-inherit force-materializes the owner's workspace before the child can attach, and concurrent creations are capped at 4 to smooth real bursts. - All sandbox containers now carry compose project/service labels (com.docker.compose.project=electric-sandboxes) so Docker Desktop groups them under one entry and they can be stopped/deleted together (docker compose -p electric-sandboxes down). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rewrite the changeset as a user-facing release note: lead with the symptom (leftover containers piling up) and the three-part fix (create-only-when-used, clean-up-on-quit, reclaim-at-startup), dropping implementation jargon. Matches the rewritten PR description; no code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
msfstef
force-pushed
the
worktree-zombie-sandbox-containers-fix
branch
from
f409434 to
8011831
Compare
Claude Code ReviewSummaryThis PR closes three gaps that let What's Working Well
Issues FoundCritical (Must Fix)None found. Important (Should Fix)None. Suggestions (Nice to Have)
Issue ConformanceNo linked issue is referenced — a process note rather than a blocker, given the unusually thorough PR description (problem statement, root-cause breakdown, fix rationale, explicit out-of-scope/follow-ups). The new image-pull commit is well-scoped and includes its own changeset ( Previous Review StatusTwo new commits since iteration 3 (
Both previously-open inline threads (kevin-dp) are resolved: the Review iteration: 4 | 2026-06-09 |
From the Claude bot review (all non-blocking nice-to-haves):
- Boot sweep now probes + reclaims leftovers concurrently (Promise.all) instead
of one awaited exec round-trip per orphan — keeps boot latency flat when many
leftovers have accumulated (the sweep is awaited before profiles build).
- Document that reclaimDockerSandboxByKey / shutdownAllDockerSandboxes are
best-effort: an unreachable daemon is swallowed and left to the next boot
sweep; note why the shutdown flush runs after the wake drain (it only reclaims
idle, lease-free containers).
- Add a lazySandbox unit test for dispose({reclaim}) racing an in-flight factory
that then fails — the reclaim callback must still run.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kevin-dp
approved these changes
Jun 9, 2026
From kevin-dp's review: replace the try/catch wrapper around the
best-effort owner-marker write with `.catch(() => {})`. Kept `await`
(not a bare `return runOneOff(...).catch(...)`) so the non-void
RunOneOffResult doesn't leak into the Promise<void> return type.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…akiness The docker sandbox test files spin up real containers, but only sandbox-docker.test.ts cleaned up after itself. The smoke and conformance suites relied solely on `sandbox.dispose()`, which doesn't remove a container synchronously — it schedules teardown ~2 minutes out via an unref'd timer that dies with the vitest process. So running those suites left RUNNING containers behind that accumulated across runs (3 smoke runs ⇒ 2 live zombies; one conformance run ⇒ 18). Lift the label-scoped sweep into a shared `installDockerSandboxTestCleanup()` helper (beforeAll + afterEach + afterAll) and call it from every describe that creates containers. The sweep filters only on the `electric-test-sandbox` test label, never the production `com.electric.sandbox` label, so a developer's real local sandboxes are untouched; it's self-gating and swallows daemon errors. The main file's duplicated sweep and six inline hooks collapse into one call. Also give the docker conformance provider a 60s timeout (matching the dedicated docker test files) — real container creation outgrows vitest's 5s default, which was flaking those tests independently of the leak. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`pullIfMissing` is documented as pulling "when it's not present locally", but `ensureImage` never checked presence — it called `docker.pull()` on every container create. `docker.pull` round-trips to the registry even for a fully cached digest-pinned image, so creation was needlessly slow and failed whenever the registry was briefly unreachable (an intermittent TLS-handshake timeout to registry-1.docker.io surfaced this). Inspect the image first (a local-only check) and pull only when it's actually absent, matching the documented semantics. Adds `getImage` to the minimal `Dockerode` interface and a unit test (fake daemon) covering present ⇒ skip, absent ⇒ pull, and `pullIfMissing: false` ⇒ neither. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Opening the desktop app could leave 15+ leftover
electric-sbx-*Docker containers running that the user never started. They piled up because, in several places, a sandbox container could outlive the work it was created for. This PR closes each of those gaps so a container only exists while something is actually using it.Why the containers piled up
Sandbox containers are meant to be short-lived: created for an agent's work, then torn down. Three gaps let them survive instead:
Two smaller bugs made it worse: a container could be stranded if its one-time setup step failed, and a Docker error code was occasionally misread so we'd "reconnect" to a container that had already been removed.
How we fix it
The approach attacks all three gaps so a container can't outlive its purpose:
As a quality-of-life touch, every container is also labelled so Docker Desktop groups them under one collapsible
electric-sandboxesentry you can stop or delete together (docker compose -p electric-sandboxes down).Implementation details (for reviewers)
Zombie reclamation
com.electric.sandbox.owner-pidlabel; the boot sweep probes it (kill(pid, 0)) and reclaims running orphans whose owner is dead — remove ephemeral, stop persistent (writable layer survives for reattach by key). The sweep probes + reclaims leftovers concurrently so boot latency stays flat as they accumulate./tmp/.electric-sbx-owner-pid, tmpfs ⇒ wiped on stop); the sweep probes it before reclaiming, so a live sibling's adopted container is never swept. The sweep is now awaited at boot so it can't race the first wake's reattach.shutdownAllDockerSandboxes()flushes pending debounced teardowns on runtime shutdown, wired throughAgentHandlerResult.shutdownSandboxes→BuiltinAgentsServer.stop()(covers desktop quit, runtime restarts, CLI SIGINT/SIGTERM; bounded 5s). Live-leased entries are left to their own dispose (a sibling runtime in the same process may own them) — if the process dies first, the pid-sweep reclaims them at next boot.mkdirexit code and force-removes the container on any failure;isNameConflict()now matches the daemon's actual name-conflict message instead of bare 409.Lazy sandbox creation
lazySandbox()wrapper (agents-runtime/sandbox/lazy.ts) defers the provider factory until the sandbox is actually used (exec/fs/fetch). The bootstrapdockerprofile returns it, so trivial wakes never create a container. Materialization is single-flight and retried on failure.reclaimDockerSandboxByKey()wipes an earlier wake's persistent workspace by key without creating a container just to delete it (owner leases only; defers to live sibling leases — the last one draining wipes it).inheritforce-materializes the owner's workspace (ensureSandboxMaterialized) before spawning, so a child can attach even when the parent never ran a tool.withCreationSlot) — bursts queue against the daemon instead of stampeding it; reattaches/execs are unlimited, total creations unbounded.Grouping
All sandboxes carry
com.docker.compose.project=electric-sandboxes(+com.docker.compose.service=<entity-type>).Testing
lazySandbox— written first, confirmed red, then green.tscand eslint clean.Out of scope (follow-ups)
e2bremote profile stays eager (working directory not statically known at profile-build time); the same wrapper applies once it is.dispatchRecoveryIntervalMsis defined but unused) — expired runner leases still queue wakes forever.🤖 Generated with Claude Code